Skip to content

[Fix] fix make_coord to require tuple arg and add crd2idx tests#251

Closed
jli-melchior wants to merge 2 commits intomainfrom
fix/crd2idx
Closed

[Fix] fix make_coord to require tuple arg and add crd2idx tests#251
jli-melchior wants to merge 2 commits intomainfrom
fix/crd2idx

Conversation

@jli-melchior
Copy link
Contributor

@jli-melchior jli-melchior commented Mar 20, 2026 •
edited
Loading

Motivation

Fixes #224

make_coord((r, c)) (passing a tuple) causes a C++ assertion failure (coord.rank() == shape.rank()) in layoutCrd2IdxTTT because *args wraps the tuple into ((r, c),) — a rank-1 nested IntTuple instead of a flat rank-2 one. Users following the layout_system_guide.md documentation hit this crash with no helpful error message.

Technical Details

  • python/flydsl/expr/primitive.py: Keep make_coord(*coord) varargs signature (consistent with make_shape and make_stride), but auto-unwrap when a single tuple argument is passed. This makes both calling conventions work identically:
    • make_coord(r, c) — varargs form (original)
    • make_coord((r, c)) — tuple form (now also supported)
  • tests/unit/Layout/test_crd2idx.py: Add 6 pytest-compatible tests:
    • test_crd2idx_col_major_tuple — verifies make_coord((r, c)) tuple form
    • test_crd2idx_col_major_varargs — verifies make_coord(r, c) varargs form
    • test_crd2idx_col_major_both_forms_equal — verifies both forms produce identical results
    • test_crd2idx_row_major — row-major (4,8) layout
    • test_crd2idx_1d — 1D layout
    • test_crd2idx_3d — 3D layout

Test Plan

FLYDSL_RUNTIME_ENABLE_CACHE=0 PYTHONPATH=./ pytest tests/unit/Layout/test_crd2idx.py -v

Test Result

tests/unit/Layout/test_crd2idx.py::test_crd2idx_col_major_tuple PASSED   [ 16%]
tests/unit/Layout/test_crd2idx.py::test_crd2idx_col_major_varargs PASSED [ 33%]
tests/unit/Layout/test_crd2idx.py::test_crd2idx_col_major_both_forms_equal PASSED [ 50%]
tests/unit/Layout/test_crd2idx.py::test_crd2idx_row_major PASSED         [ 66%]
tests/unit/Layout/test_crd2idx.py::test_crd2idx_1d PASSED                [ 83%]
tests/unit/Layout/test_crd2idx.py::test_crd2idx_3d PASSED                [100%]

6 passed in 16.41s

Submission Checklist

  • Code compiles and passes existing tests
  • New tests added for the changed behavior
  • No breaking changes — make_coord(r, c) still works, make_coord((r, c)) now also works
  • Consistent with make_shape and make_stride API design

@jli-melchior
[Fix] fix make_coord to require tuple arg and add crd2idx tests
3de9092 
Change make_coord(*coord) to make_coord(coord) so that passing
multiple args like make_coord(r, c) raises a clear TypeError instead
of silently creating a wrong-rank coordinate. Update the one caller
in kernels/layout_utils.py and add comprehensive pytest-compatible
tests covering col-major, row-major, 1D, 3D layouts and error cases.
@jli-melchior jli-melchior requested a review from coderfeli March 20, 2026 03:40
@jli-melchior jli-melchior changed the title fix make_coord to require tuple arg and add crd2idx tests [Fix] fix make_coord to require tuple arg and add crd2idx tests Mar 20, 2026
@jli-melchior jli-melchior mentioned this pull request Mar 20, 2026
Open
sjfeng1999
sjfeng1999 reviewed Mar 20, 2026
View reviewed changes
python/flydsl/expr/primitive.py Outdated

@traced_op
def make_coord(*coord, loc=None, ip=None):
def make_coord(coord, loc=None, ip=None):
Copy link
Contributor

@sjfeng1999 sjfeng1999 Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make_coord doesn't only accept a tuple. It is the same as make_shape and make_stride. There is no need to change.

Copy link
Contributor Author

@jli-melchior jli-melchior Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we can let make_coord not only accept a tuple, but also a variable-length arguments?

@jli-melchior @claude
keep make_coord *args and support both tuple and varargs forms
01e66c3 
Revert make_coord to *args signature (consistent with make_shape and
make_stride), but auto-unwrap a single tuple argument so both
make_coord(r, c) and make_coord((r, c)) produce the same result.
Update tests to verify both calling conventions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderfeli coderfeli requested a review from xudoyuan March 20, 2026 14:58
@coderfeli
Copy link
Collaborator

coderfeli commented Mar 24, 2026

@jli-melchior shall we close this pr?

@jli-melchior
Copy link
Contributor Author

jli-melchior commented Mar 24, 2026

@jli-melchior shall we close this pr?

Yes, we can close this PR for now because fx.make_coord is designed to accept variable-length arguments instead of a tuple.With this PR, we would support fx.make_coord((r, c)), fx.make_coord(*(r, c)), and fx.make_coord(r, c).Without this PR, we can still support fx.make_coord(*(r, c)) and fx.make_coord(r, c).While this comes down to user preference, closing the PR allows us to avoid compilation errors by requiring users to write the same rank as the layout.

@coderfeli coderfeli closed this Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sjfeng1999 sjfeng1999 sjfeng1999 left review comments

@coderfeli coderfeli Awaiting requested review from coderfeli

@xudoyuan xudoyuan Awaiting requested review from xudoyuan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Issue]: fx.crd2idx not working with coord returned fx.make_coord

3 participants

@jli-melchior @coderfeli @sjfeng1999